fix: parse XML thinking and tool_call blocks in OpenRouter responses #6634
Conversation
- Add XML parsing for <think> and <tool_call> blocks in OpenRouter handler - Handle incomplete XML blocks across streaming chunks - Convert tool_call blocks to user-friendly format - Add comprehensive tests for XML parsing functionality Fixes #6630
dosubot
bot
added
size:L
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| import { addCacheBreakpoints as addGeminiCacheBreakpoints } from "../transform/caching/gemini" | ||
| import type { OpenRouterReasoningParams } from "../transform/reasoning" | ||
| import { getModelParams } from "../transform/model-params" | ||
| import { XmlMatcher } from "../../utils/xml-matcher" |
There was a problem hiding this comment.
Remove unused import 'XmlMatcher' if it's not needed to avoid confusion.
| import { XmlMatcher } from "../../utils/xml-matcher" |
This comment was generated because it violated a code review rule: irule_Vw7dJWzvznOJagxS.
| yield { type: "text", text: delta.content } | ||
| buffer += delta.content | ||
|
|
||
| // Process complete XML blocks |
There was a problem hiding this comment.
Consider refactoring the inline XML parsing logic (lines 158–216) into a shared utility (or use the imported XmlMatcher) for improved readability and maintainability.
This comment was generated because it violated a code review rule: irule_tTqpIuNs8DV0QFGj.
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| import { addCacheBreakpoints as addGeminiCacheBreakpoints } from "../transform/caching/gemini" | ||
| import type { OpenRouterReasoningParams } from "../transform/reasoning" | ||
| import { getModelParams } from "../transform/model-params" | ||
| import { XmlMatcher } from "../../utils/xml-matcher" |
There was a problem hiding this comment.
I notice we're importing XmlMatcher but not using it. Since we've implemented custom XML parsing logic below, should we remove this unused import? Or perhaps we could consider using the existing XmlMatcher utility instead of the manual implementation?
| processed = false | ||
|
|
||
| // Check for complete <think> blocks | ||
| const thinkMatch = buffer.match(/^(.*?)<think>([\s\S]*?)<\/think>(.*)$/s) |
There was a problem hiding this comment.
The regex patterns assume well-formed XML. What happens if the model sends malformed XML like <think>content without closing tag or nested tags? The current implementation might not handle these edge cases gracefully. Should we add some validation or fallback behavior?
|
|
||
| // Process complete XML blocks | ||
| let processed = true | ||
| while (processed) { |
There was a problem hiding this comment.
For large responses with many XML blocks, this while loop with multiple regex matches could impact performance. Have we considered the performance implications? Perhaps we could optimize by combining the regex patterns or using a different parsing approach?
| } | ||
|
|
||
| // Check for complete <tool_call> blocks | ||
| const toolMatch = buffer.match(/^(.*?)<tool_call>([\s\S]*?)<\/tool_call>(.*)$/s) |
There was a problem hiding this comment.
The logic for handling <think> and <tool_call> blocks is nearly identical. Could we refactor this into a helper function to reduce duplication? Something like:
| const toolMatch = buffer.match(/^(.*?)<tool_call>([\s\S]*?)<\/tool_call>(.*)$/s) | |
| // Helper function to process XML blocks | |
| const processXmlBlock = (buffer: string, tagName: string, transform?: (content: string) => any) => { | |
| const regex = new RegExp(`^(.*?)<${tagName}>([\s\S]*?)<\/${tagName}>(.*)$`, 's'); | |
| const match = buffer.match(regex); | |
| if (match) { | |
| const [, before, content, after] = match; | |
| return { matched: true, before, content: transform ? transform(content) : content, after }; | |
| } | |
| return { matched: false }; | |
| }; |
| } | ||
|
|
||
| // Check if we have an incomplete tag at the end | ||
| const incompleteTag = buffer.match(/^(.*?)(<(?:think|tool_call)[^>]*(?:>[\s\S]*)?)?$/s) |
There was a problem hiding this comment.
This buffer management logic for incomplete tags is complex. Could we add some inline comments explaining the different scenarios? For example:
- When we have an incomplete tag at the end
- When we need to preserve partial tag content
- When it's safe to yield all content
This would improve maintainability for future developers (including myself in 5 minutes).
hannesrudolph
added
the
Issue/PR - Triage
|
Isn't this also an issue when using ollama? |
|
This is not a proper fix, the model is outputting the tag |
This PR fixes the issue where Qwen3 Coder model through OpenRouter sends thinking and tool call blocks as raw XML text instead of being properly parsed and displayed in their respective UI panes.
Problem
When using the
qwen/qwen3-coder:freemodel through OpenRouter, the response contains<think>...</think>and<tool_call>...</tool_call>blocks as raw text in the content, which are displayed directly to the user instead of being parsed and shown in the appropriate UI sections.Solution
<think>blocks to reasoning chunks that display in the thinking pane<tool_call>blocks to a user-friendly format with[Tool Call]:prefixTesting
<think>block parsing<tool_call>block parsingFixes #6630
Important
Fixes XML parsing for
<think>and<tool_call>blocks in OpenRouter responses, adding tests for various scenarios.<think>and<tool_call>XML blocks inOpenRouterHandlerinopenrouter.ts.<think>blocks to reasoning chunks and<tool_call>blocks to text with[Tool Call]:prefix.openrouter.spec.tsfor parsing<think>and<tool_call>blocks, nested/multiple XML blocks, and incomplete XML blocks.This description was created by
for aa8cb2f. You can customize this summary. It will automatically update as commits are pushed.